Fix: declare the four hoisted runtime dependencies in the dev workspace - #577
Open
AmaadMartin wants to merge 3 commits into
Open
Fix: declare the four hoisted runtime dependencies in the dev workspace#577AmaadMartin wants to merge 3 commits into
AmaadMartin wants to merge 3 commits into
Conversation
added 3 commits
August 3, 2026 08:15
dev/src imports @opentelemetry/api, @opentelemetry/sdk-trace-base, lodash-es and @google-cloud/vertexai, but dev/package.json declares none of them. They resolve in-repo only because core/package.json declares them and npm hoists them to the workspace root, so 'npm install @google/adk-devtools' outside this repo gets none of them. dev/build.js passes packages:'external' to esbuild, so every one of these specifiers survives unbundled into the published dist/esm and dist/cjs and is resolved against the consumer's tree. Each range is copied character for character from core/package.json so npm keeps collapsing both workspaces onto one physical copy. @opentelemetry/api stays an exact pin: two copies of the OTel API in one process each carry their own global tracer registry, and spans recorded against one are invisible to the other. @types/lodash-es is a devDependency because lodash-es ships no type declarations and dev's 'tsc --emitDeclarationOnly' build step needs them, but cloneDeep's types never reach dev's public .d.ts. This mirrors how core pairs the two.
@google-cloud/vertexai@1.12.0 re-exports Client from its root (build/src/index.d.ts), so the deep build-output path reached into implementation detail with no semver guarantee. Both specifiers resolve to the same file in the same package instance, so class identity is unchanged. ReasoningEngine stays on its deep path: it is declared in build/src/genai/types/common.d.ts and re-exported only by build/src/genai/types.d.ts, so the package root does not expose it. Vitest intercepts by specifier, so the deep-path vi.mock in cli_deploy_agent_engine_test.ts would have silently stopped applying and let the suite construct the real Client; the mock is retargeted to match. Its factory body is unchanged.
dev/src imports @google/genai at six sites and it was left undeclared, resolving only by hoisting from core -- the same bug class this branch fixes for the other four packages, so leaving it out shipped a fix that did not actually make dev standalone-installable. It is not type-only: createUserContent is called at dev/src/server/adk_api_client.ts:153 and survives into the published output as import_genai.createUserContent, and AdkApiClient is exported from dev/src/index.ts, so a standalone install hit ERR_MODULE_NOT_FOUND on a public entry point. The range matches core/package.json:48 so npm keeps both workspaces on one copy; npm ls confirms @google/genai@2.9.0 deduped for core and dev. The nested 1.52.0 under @google-cloud/vertexai is that package's own pin and is untouched. Added to SHARED_RUNTIME_DEPENDENCIES so the guard covers it: the list is hand-maintained, so an omission there is invisible.
AmaadMartin
pushed a commit
that referenced
this pull request
Aug 4, 2026
Replaces the Math.random() UUID fallback with crypto.getRandomValues(), and throws rather than silently degrading to a non-cryptographic generator when no secure source exists. crypto.randomUUID() is secure-context-only, so it is absent on plain-HTTP origins even where crypto is present; getRandomValues() carries no such restriction and is used as the fallback. Callers making security decisions on this value (the OAuth2 state parameter in AuthHandler, and session identifiers minted by the session services) can no longer be handed a predictable UUID. Fallback applies RFC 4122 section 4.4 version and variant bits and zero-pads every byte. Tests pin the randomUUID branch discriminatingly, cover the getRandomValues fallback deterministically, and assert the throw.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Link to an existing issue (if applicable):
N/A — no public issue is open for this.
Or, if no issue exists, describe the change:
Problem:
@google/adk-devtools(thedevworkspace) imports five npm packages that its owndev/package.jsonnever declares:@opentelemetry/apidev/src/utils/telemetry_utils.ts:13(HrTime),dev/src/server/adk_api_server.ts:29(trace,TracerProvider)traceis a runtime value (adk_api_server.ts:160); the rest are types that reach the emitted.d.ts@opentelemetry/sdk-trace-basedev/src/utils/telemetry_utils.ts:14-18,dev/src/server/adk_api_server.ts:30(SimpleSpanProcessor)SimpleSpanProcessoris a runtime value (adk_api_server.ts:153-154)lodash-esdev/src/integration/test_runner.ts:15(cloneDeep)test_runner.ts:68)@google-cloud/vertexaidev/src/cli/deploy/cli_deploy_agent_engine.ts:9-10Clientis a runtime value (:137)They resolve inside the monorepo only because
core/package.jsondeclares them and npm hoists them to the repo-rootnode_modules. A user runningnpm install @google/adk-devtoolsoutside this repo gets none of them.The failure is invisible in-repo, which is why it survived. I verified the mechanism rather than assuming it:
dev/build.jspassespackages: 'external'to esbuild, so nothing is bundled. Counting specifier occurrences in the packed tarball's output confirms every one survives to be resolved against the consumer's tree:Solution: Declare all five in
dev/package.json, copying each range character for character fromcore/package.json(the source of truth) so npm keeps collapsing both workspaces onto one physical copy:All five go in
dependencies, notdevDependencies: each contributes a runtime value to published output, and the OTel types additionally appear indev's emitted.d.ts— verified in the packed tarball, e.g.dist/types/server/adk_api_server.d.ts:7importsTracerProvider."@opentelemetry/api"is deliberately left un-caretted to match core's pin. This is load-bearing, not cosmetic: two copies of the OTel API in one process each carry their own global tracer registry, and spans recorded against one are invisible to the other. A test pins this exact-pin invariant (mutation 2 below).A fifth manifest line beyond the four audited packages:
@types/lodash-es: ^4.17.12indevDependencies.lodash-es@4.18.1ships no type declarations of its own, sodev'stsc --emitDeclarationOnlybuild step cannot typechecktest_runner.tswithout it. It is a devDependency becausecloneDeep's types never surface indev's public.d.ts— mirroring howcorepairs the two (core/package.json:68+:77).Secondary change —
Clientmoved to the package root. Being precise about what this does and does not achieve, since it is the one hunk in this PR that is not strictly required to declare a dependency:@google-cloud/vertexai@1.12.0has noexportsmap at all, so the deep pathbuild/src/genai/client.jsresolved fine before and would continue to, hoisted or not. Nothing was broken.@google-cloud/vertexai) instead of deep paths."build/src/index.d.tsdoesexport { Client } from './genai/client', so the root exposesClient, and all threeClientimporters incorealready use it. Both specifiers resolve to the same file in the same package instance, so class identity is unchanged.ReasoningEngine, which is declared inbuild/src/genai/types/common.d.tsand re-exported only bybuild/src/genai/types.d.ts— the package root genuinely does not expose it. Two deep imports become one. If you would rather this PR stayed purely about manifests, these two hunks are separable and can be dropped without affecting anything else here; Fix: drop @google-cloud/vertexai deep build-output imports from the Agent Engine deploy CLI #279 also covers this rewrite.One-line mock retarget, and why it is not optional. Vitest intercepts by specifier, so once the source imports the root, the deep-path
vi.mockatdev/test/cli/cli_deploy_agent_engine_test.ts:144silently stops applying and the suite constructs the realClient. I retargeted that one specifier and left the factory body and every assertion untouched. This is a mechanical consequence of the source change, not a rewrite of assertions — and mutation 3 below shows it is load-bearing rather than cosmetic: leaving the mock on the deep path fails 12 of 17 tests with live 403s against the Agent Platform API.Collision check (done before writing any code).
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000(the fork has ~380 open PRs; smaller limits silently truncate). Adjacent open PRs found, and how this one relates:dev/package.json. Fix: declare dev workspace's undeclared runtime dependencies #249 is CONFLICTING; Fix: declare dev's undeclared runtime dependencies and prefix node:path #485 is a superset that also adds@google/genaiand editsdev/src/cli/cli.ts.@opentelemetry/apionly. Fix: declare hoisted runtime imports in dev/core and enforce with import/no-extraneous-dependencies #541 declares them and adds animport/no-extraneous-dependencieslint gate.I initially stopped as a duplicate; that was overruled on the grounds that none of the above is a byte-exact match for this change's scope. Whoever merges these should expect a textual conflict in the alphabetically-sorted
dependenciesblock and inpackage-lock.json, resolvable by keeping both sets of keys in alphabetical order and re-runningnpm install.Scope note —
@google/genaiis now included. The original plan for this change deliberately excluded it, on the grounds that a sibling PR (#244) owns it. A complexity review pushed back on that, correctly: leaving it out shipped a fix that did not actually achieve the stated goal. It is an undeclared runtime dependency of exactly the same kind — 6 import sites,createUserContentcalled atdev/src/server/adk_api_client.ts:153(and present in the published output asimport_genai.createUserContent), reached throughAdkApiClient, which is a public export atdev/src/index.ts:7. A standalone install gotERR_MODULE_NOT_FOUNDon a public entry point. Declaring four of five packages would have left the bug live while looking fixed, so it is declared here at core's^2.9.0.A repo-wide phantom-dependency guard is still not part of this PR — that is a different mechanism (walking every workspace's
srcand diffing against its manifest) and is already open separately as #450 and #250. The list inpackage_manifest_test.tsis hand-maintained and covers what this PR is responsible for; the same review noted a hardcoded list can go stale, which is precisely what the automated guard in #450 is for.Resolved versions after
npm install(read out ofpackage-lock.json;npm lsreports every one asdeduped, i.e.coreanddevshare the single hoisted root copy rather than each having their own):@opentelemetry/apinode_modules(shared by root,core,dev)@opentelemetry/sdk-trace-basenode_modules(shared by root,core,dev)lodash-esnode_modules(shared by root,core,dev)@google-cloud/vertexainode_modules(shared by root,core,dev)@types/lodash-esnode_modules(shared by root,core,dev)npm installcreated no new nesting: thepackage-lock.jsondiff is confined to the six added keys inside the"dev"entry under"packages", and adds/changes zeronode_modules/...entries. (Pre-existing nested copies that are other packages' own pins are untouched:@opentelemetry/sdk-trace-base@2.1.0under@opentelemetry/otlp-transformerand@opentelemetry/exporter-trace-otlp-http, and@google/genai@1.52.0under@google-cloud/vertexai. Deduping that last one is its own concern, open separately as #274/#515.)No behavioral change to any shipped code path. Not a breaking change — packages that previously had to be present by accident are now requested explicitly, and every declared range is already satisfied by the installed copy, so existing monorepo users see no version movement.
Testing Plan
Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
New test
dev/test/package_manifest_test.ts(6 cases) pins each range againstcore's. It resolves both manifests fromimport.meta.urlrather thanprocess.cwd(), so it does not depend on vitest running from the repo root, and it asserts core's value istoBeDefined()before the equality assertion — otherwiseundefined === undefinedwould pass vacuously if both manifests lost the key.This change adds zero new executable lines under any
*/src/**, so the v8 coverageincludeglobs see no new code and the global thresholds invitest.config.tsare unaffected. No threshold was lowered or adjusted.Proof each test can fail. Coverage is not proof, so I ran four mutations and confirmed each produces a real failure:
"@opentelemetry/api": "1.9.0"fromdev/package.json:"^1.9.0"— proving the test pins the exact-pin invariant and not merely presence:vi.mockon the root, to prove the mock retarget is load-bearing. 12 of 17 tests failed and the suite tried to reach the live Agent Platform API, exactly the silent-passthrough hazard the retarget prevents:"@google/genai": "^2.9.0":No existing test was deleted, skipped, weakened, or had an assertion changed.
Manual End-to-End (E2E) Tests:
Packaging check — the actual user-visible consequence:
Repo-root verification:
One honest caveat:
npm run ts:checkexits 2, but it does so identically on the clean baseline. I verified this rather than assuming — stashing this change and re-running produces a byte-identical 41-line error list, none of it in any file this PR touches. It is a pre-existing repo condition (and is not one of the CI steps in.github/workflows/validation.yaml, which runsnpm install,secretlint,build,test:coverage,lint,format:check,docs:check).CI status (reported honestly)
Head commit
37aaee66. Four of five checks green;run-tests (windows-latest)is red and I am not calling this run green.check-licenserun-testsrun-tests (ubuntu-latest)✓ dev/test/package_manifest_test.ts (6 tests)run-tests (macos-latest)run-tests (windows-latest)core/test/code_executors/unsafe_local_code_executor_test.ts > UnsafeLocalCodeExecutor > should execute shell code and return stdout,Error: Test timed out in 5000msBoth failures seen on this branch are pre-existing CI flakes, not regressions from this change. I verified that rather than asserting it:
feat/application-integration-toolset-part1(nothing to do with dependencies) failed at 16:25Z with the identicaltests/integration/app_loader/app_loader_test.ts … Error: Test timed out in 40000ms. The same failure also occurs on forkmain— runs30405795959and29450120872. It has since passed here on re-run.mainrun28808663759shows the sameTest timed out in 5000msclass on windows, and five open PRs target this exact suite (Fix: give the real-subprocess cases in unsafe_local_code_executor_test.ts an explicit 60s timeout #498, Fix: decode child stdout/stderr with setEncoding in UnsafeLocalCodeExecutor #373, Fix: create the UnsafeLocalCodeExecutor scratch directory atomically with fs.mkdtemp #355, Fix: harden Windows shell cases in unsafe_local_code_executor unit tests #254, Fix: give subprocess-spawning tests an explicit 60s timeout #224).unit:coretest that spawns a shell to runecho "Hello, Shell!"and asserts on stdout within 5s — subprocess-spawn latency on a loaded runner.dev/package.jsonis not in its module graph. The delta from the commit where all three OSes passed is three lines: onedev/package.jsonentry, one entry in the test's dependency list, one lockfile line.shellonly, thenpython+shell, thenshellonly), which is the signature of resource contention rather than a deterministic break.I deliberately have not touched those suites to force a green tick. Raising their timeouts is a separate concern with existing PRs, and bundling it would add unrelated churn to a dependency-declaration diff. Local validation on this exact commit: 23/23 targeted tests,
npm run build,lint,format:check,docs:checkandsecretlintall pass.Checklist
[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.